[16.0][IMP] account_statement_import_online: Pull wizard with dates and one fetch#798
Conversation
|
Hi @alexey-pelykh, |
pedrobaeza
force-pushed
the
16.0-imp-account_statement_import_online-pull_wizard
branch
from
3cd8590 to
b099bdc
Compare
| statement_date_until = ( | ||
| statement_date_since + provider._get_statement_date_step() | ||
| ) | ||
| if one_fetch: |
There was a problem hiding this comment.
How checking this by default is compatible with providers that limit fetch to X days per fetch?
There was a problem hiding this comment.
Can you put any example of that limit? All that I know don't have such limit. And anyway, if you put a big interval, you get in return the message error if so, so you can limit your interval in the wizard. Remember that this is only for the interactive pull wizard.
There was a problem hiding this comment.
E.g. PayPal - one year only, Wise.com - one year only. User may as well select 5 years when importing for the first time on new setup. Such providers should set the step to 1 year making the one_step setting irrelevant.
Obviously you're aiming to fix experience for some specific providers - what step do they set? Since if they set one day but can handle much more so that one_fetch works - those providers' step should be fixed and not an override added.
There was a problem hiding this comment.
Yeah, and if they do that, an error will be arisen from the Paypal provider, so I don't see any problem. Instead, with the default value that is daily, they will make 365*5 = 1825 requests, taking a lot of time and make them to think that it's hung.
We can put a warning if you prefer saying that the provider may not accept the petition in one block, but what is not sane is to not do it by default.
There was a problem hiding this comment.
Let's use just one of the two threads :D why adding statement_creation_mode like all was not an option?
There was a problem hiding this comment.
Because you want to do this only once, not for every automatic pull that happens once you do the initial pulling, that is only needed to be updated. Another possible use is any temporary problem that prevented to download an interval, and in such case, you also want only one pull, not one by one. Example: during a week, you have invalid credentials. Once you put the valid ones, having create statements as daily, only last day will be downloaded, so you click on the pull wizard and download the last week. Why would you want to do this by default through 7 requests instead of just one?
There was a problem hiding this comment.
Let's pause for a second to investigate the idea behind statement_creation_mode having day/week/month:
By configuring statement_creation_mode on the provider to day/week/month we control how many statements area created, or in other words - how transactions are grouped.
If I got your argument for one_fetch correctly, it is proposed to solve the API rate limiting. As a side effect it overrides the statement_creation_mode, creating a single statement from all. I definitely see issue with that.
In your example, with daily setting, and one_fetch checked - one statement covering entire period will be generated.
I fully acknowledge the API rate limiting issue that may arise for some providers when you pull yearly data with day granularity. However I strongly disagree with the proposed approach.
As a counter-proposal, I'd suggest:
- adding
_get_statement_date_step_max()(None by default) so that providers like PayPal may set it to 365 days. - refactoring the code to use
_get_statement_date_step_max(effectively hardcodingone_fetchbehavior) for fetch - refactoring code to step through the downloaded data according to
statement_creation_mode
that would allow to fetch data in minimal amount of requests (one for some providers, couple for providers like PayPal) while respecting the statement grouping granularity of day/week/month
… fetch This commit improves the provider pull wizard in 2 ways: - It asks for dates instead of datetimes, as at the end, the minimum pull unit is the whole day, and it was very confusing for the user to select times without knowing that behind the scenes, the whole day will be pulled. Now, the user selects only the day. Quicker and cleaner... - It includes a check "One fetch", that instructs the pull method to ask to the provider the whole interval in one shot. This way, you don't consume the request rate limits for the providers with them, or only due to speed purposes. It's checked by default, as it's the more natural way of fetching an interval on demand. TT55399
pedrobaeza
force-pushed
the
16.0-imp-account_statement_import_online-pull_wizard
branch
from
b099bdc to
0c111d8
Compare
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
github-actions
Bot
added
the
stale
victoralmau
left a comment
victoralmau
left a comment
There was a problem hiding this comment.
IMO this change is coherent.
pedrobaeza
removed
the
stale
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
github-actions
Bot
added
the
stale
pedrobaeza
removed
the
stale
alexey-pelykh
left a comment
alexey-pelykh
left a comment
There was a problem hiding this comment.
Review: account_statement_import_online Pull Wizard Improvements
Thanks for the contribution, @pedrobaeza. The Date-vs-Datetime UX improvement is valuable. However, the one_fetch mechanism raises architectural concerns that merit further discussion before merging.
What works well
- Date fields instead of Datetime: Good UX improvement. Users selecting dates is more intuitive when the minimum pull granularity is a full day.
_get_datetimes()helper: Clean conversion method with propermin.time()/max.time()boundaries.- Ponto override propagation: The
_pullsignature change is correctly forwarded throughsuper()in the Ponto provider.
Design concerns with one_fetch
-
Bypasses
_get_statement_date_since: Whenone_fetch=True, the method overridesstatement_date_since = date_sinceat line 194, skipping the provider-specific logic in_get_statement_date_since()that may adjust the start date (e.g. for providers that need alignment to statement boundaries). This silent override could produce unexpected results for providers that rely on that adjustment. -
Bypasses
_get_statement_date_stepgrouping: Whenone_fetch=True,statement_date_until = date_untilat line 208 means a single call to_obtain_statement_datafor the entire period. The resulting statement will cover the full date range regardless ofstatement_creation_mode(daily/weekly/monthly). This is a side effect -- the user configures daily statements but gets one big statement when pulling interactively. -
Timezone concern in
_get_datetimes(): The method produces naive datetimes viadatetime.combine(date, datetime.min.time()). Whenone_fetch=True, the normal path through_get_statement_date_since(which handles timezone-aware date alignment) is skipped. This could cause subtle off-by-one-day issues for providers in non-UTC timezones. -
Default
Trueis risky: Defaultingone_fetch=Truemeans the wizard will by default bypass the carefully designed step-based iteration for all providers, including future ones that may depend on it. A provider author implementing_get_statement_date_stepto return 30 days (because their API limits requests to 30-day windows) would find that the wizard silently ignores their constraint.
Alternative approach suggestion
As discussed in the earlier review thread, a cleaner architecture might be:
- Add
_get_statement_date_step_max()returningNoneby default (unlimited) - Providers with API limits (PayPal, Wise) override it to their max window
- The fetch loop uses
_get_statement_date_step_maxfor fetching but still creates statements perstatement_creation_mode
This separates the two concerns: fetch granularity (how many API calls) vs statement grouping (how transactions are organized).
Test coverage
The GoCardless test is updated to set one_fetch=False, preserving existing behavior. However, there is no test exercising the one_fetch=True path to verify it actually works correctly (single fetch, statement boundaries, etc.).
Verdict
The Date field change is ready to merge on its own. The one_fetch mechanism needs design refinement to avoid bypassing provider-specific constraints and statement grouping configuration. Consider splitting this into two PRs: one for the Date UX improvement, and one for the fetch optimization after the architectural approach is agreed upon.
Review posted via CorporateHub OCA review campaign
| """Return the datetime values for the entered dates, including whole starting | ||
| and ending days. | ||
| """ | ||
| self.ensure_one() |
There was a problem hiding this comment.
Timezone concern: datetime.combine(self.date_since, datetime.min.time()) produces a naive datetime (no tzinfo). When one_fetch=True, this value is used directly as date_since in _pull, bypassing _get_statement_date_since() which normally handles date alignment. For providers operating in non-UTC timezones, this could cause off-by-one-day boundary issues.
Consider using fields.Datetime.to_datetime() or applying the provider's timezone to ensure consistency with the normal code path.
| def _pull(self, date_since, date_until, one_fetch=False): | ||
| """Pull data for all providers within requested period.""" | ||
| is_scheduled = self.env.context.get("scheduled") | ||
| debug = self.env.context.get("account_statement_online_import_debug") |
There was a problem hiding this comment.
Design concern: This overrides _get_statement_date_since() which providers may use to align the start date to statement boundaries (e.g., start of week/month for weekly/monthly statement creation modes). Bypassing it means the fetched data may not align with the provider's expected statement boundaries.
The _get_statement_date_since method exists to allow providers to customize this behavior -- silently skipping it via a boolean flag breaks that contract.
| statement_date_since + provider._get_statement_date_step() | ||
| ) | ||
| if one_fetch: | ||
| statement_date_until = date_until |
There was a problem hiding this comment.
Statement grouping side effect: Setting statement_date_until = date_until here means a single iteration of the while loop, producing one statement for the entire period. This overrides statement_creation_mode (daily/weekly/monthly) -- the user configures daily statements but gets a single multi-day statement when using the wizard with one_fetch checked.
Fetch optimization and statement grouping are separate concerns that should not be coupled through a single flag.
4 participants
This commit improves the provider pull wizard in 2 ways:
@Tecnativa TT55399